-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for tags on AWS resources #617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your awesome contribution! Let me ask some questions.
First of all, what do you think about integrating these tags rules together? In my understanding, these rules don't have a difference for each resource, so implementing them as a single rule is better in terms of maintainability.
Secondly, What do you think about implementing tags config as rule configurations? The attributes of the top-level config block should describe the overall behavior, and I want to write the configuration for a particular rule in a per-rule block. e.g:
rule "aws_resource_tags_aws_accessanalyzer_analyzer" {
tags = ["foo", "bar"]
}
Unfortunately, there is currently no way to write a config for each rule, so we have to do it first, but I think it will be better.
I actually implemented it this way first, but I wasn't able to import
Debugging this led me to this doc, which suggested that maybe depending on the provider in
I thought about this too since the top level config block doesn't seem like the right place for an AWS-specific configuration. However, it strikes me as a terrible user experience to define the tags for each type of resource individually in a
This would allow for additional AWS-specific resource configs in the future. Also, should this continue to allow |
In the most recent committed I've updated the code generation to create a list of the types of resources that have tags, rather than a duplicated rules file for each type. This seems much cleaner. What do you think of the idea of a top-level |
Great. In addition, it is a good idea to make this list in
This problem may be solved by not defining rules for each resource. I think there is no reason to split rules for each resource. What do you think? |
This is a good idea! I will work on it tomorrow. 👍🏻 |
Oh, okay, I'll wait for that to be merged then. I have the single rule version ready to go now. Just need to know how to load the config. Will you update the docs to explain the change in #624? I don't quite follow how it will work yet. |
Maybe this feature is as simple as no need to update the developer guide. You just need to make the struct of the configuration you want to reference in the rule and pass it to The only caveat is that the struct must have tags for |
I've incorporated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left some comments about the behavior of the rule, please check them.
I've incorporated your feedback. Given a resource like this:
and a configuration like this:
it produces an error like this:
Note that in order to access |
Umm... you are right. the current API has no way to access the resource details. Temporarily creating a similar API is not a bad idea, but given the current situation, it might be a good idea to substitute an ambiguous message like "The resource". What do you think? |
Well I did like your suggestion to reference which resource was missing tags. Without it, the user will have to search through all the code to find which resource is missing them. A few options come to mind:
|
In my opinion, the location (file name, line number) is always displayed, so the resource name is not always important. So I would like to suggest 1. |
Okay, let me know if you have any further comments. |
👍 |
This PR adds code generated rules for checking resources for named AWS tags.
In config, you can specify a
tags
attribute in theconfig
block:All the AWS resources with
tags
supported will be checked for tags matching these keys.This addresses #150.